Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates to reflect service worker registration being opt-in. #3924

Merged
merged 4 commits into from
Sep 19, 2018

Conversation

jeffposnick
Copy link
Contributor

R: @gaearon @Timer etc.

Fixes #3864

There are some changes to the docs needed if/when navigateFallback is re-enabled in the service worker configuration, but those are tracked in #3870 and I'm guessing that @piotr-cz will tackle them?

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@jeffposnick
Copy link
Contributor Author

(Re: the CLA, it has been previously addressed outside of the normal submission process.)

@@ -74,8 +74,8 @@ You can find the most recent version of this guide [here](https://github.com/fac
- [Getting Started with Storybook](#getting-started-with-storybook)
- [Getting Started with Styleguidist](#getting-started-with-styleguidist)
- [Publishing Components to npm](#publishing-components-to-npm)
- [Making a Progressive Web App](#making-a-progressive-web-app)
- [Opting Out of Caching](#opting-out-of-caching)
- [Making a Progressive Web App](#making-Making a Progressive Web Appa-progressive-web-app)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops

@Timer Timer added this to the 2.0.0 milestone Jan 26, 2018

>Note: In the current version of `create-react-app`, opting-in is required for
all developers who want offline-first behavior, even if you've previously
deployed a version of your web app which used a service worker. If you redeploy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right place to say redeploy because the opt-in only affect the people who generate a new project with CRA. The people that are upgrading to v2 from existing v1 generated project would still have the old registration code. (if they don't also update the file manually, it is)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so the local src/index.js is left alone when someone updates create-react-app from v1 to v2? I've got to be honest that I'm not familiar with the upgrade process 😄

If that's the case, I'll update the text. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, all files in src are left alone when upgrading. "Upgrading" is really just updating a dependency in package.json. The template code in src is only used when creating a new project.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usually, there would be a guide in the release like https://github.com/facebook/create-react-app/releases/tag/v1.0.8 to migrate the files in src, which I think is the more appropriate place for it.

@piotr-cz
Copy link
Contributor

#3870 is just revert of #3419

@piotr-cz
Copy link
Contributor

piotr-cz commented Jan 26, 2018

@jeffposnick I think it'll be more appropriate if I'd close #3870 and you'd handle the situation in one PR (revert of #3419 and updating the docs) if that's ok with you.

I was about to resolve conflicts in my PR but this would just create new ones in this one.

@jeffposnick
Copy link
Contributor Author

@piotr-cz—I can handle that all in a single PR, sure.

@piotr-cz
Copy link
Contributor

piotr-cz commented Feb 2, 2018

@jeffposnick Thanks, I've closed #3870

@jeffposnick
Copy link
Contributor Author

I've moved @piotr-cz's changes from #3870 (reinstating navigateFallback) into this PR.

I've also added a config option to disable skipWaiting, as it leads to better compatibility with lazy-loading at the expense of potentially extended the lifetime of previously cached data (until you close the final window/tab that had an old version of your web app open). Since a developer who explicitly opts-in to service worker support is less likely to be confused about showing cached data, it seemed like a good compromise.

@jliebrand
Copy link

Hey guys - any update on this PR?

@psr-ai
Copy link

psr-ai commented Mar 23, 2018

Hey everyone, any estimate when this will be merged?

@Timer Timer merged commit 1b28131 into facebook:next Sep 19, 2018
zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
…#3924)

* Updates to reflect service worker registration being opt-in.

* Fixed an anchor link.

* Updates to SWPrecacheWebpackPlugin config, and corresponding docs.
@lock lock bot locked and limited conversation to collaborators Jan 19, 2019
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants